-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add commands function #64
Conversation
b20eff3
to
fc6c21b
Compare
Adds new `command` function that returns the underlying `std::Command` that would be called to open given path. Related: Byron#63 Related: helix-editor/helix#5820
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for giving it a shot!
Instead of trying --version
in various programs, I think it's better to try to execute them. That way, Command
tells you if it could find an executable by that name by erroring when trying to launch it, or not. This assumes these programs don't have a side-effect when launched like that which I believe should be the case. That, however, definitely needs validation.
Once a program is found, one could cache this information statically, but then again, we didn't do that previously and can probably just keep everything pure and simple. In the worst case, we would probably waste 40ms to find a program that works, which seems quite alright.
@Byron done. Any recommendations for how to easily fix this for someone who runs solely on macos devices? I could spin up a few VMs but maybe there's easier way? |
I think we actually should neither call version or execute the command in anyway. Instead we should just return a vec of commands that can be executed by the calle until one of then works. Calling any command in anyway is a blocking operation and should not be done unless explicitly requested. |
I am using Parallels for that, and am not aware of an easier way except for using CI, but that has its own drawbacks. Once the API is finalized I am happy to help test it on some linux VMs I have available, and windows as well. |
Fair enough! Let's not return a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for getting this work started!
I have added a few localized comments which I hope are helpful.
src/haiku.rs
Outdated
.arg(path.as_ref()) | ||
.status_without_output() | ||
.into_result() | ||
command(path).status_without_output().into_result() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this CommandExt
method should be split into two, so the configuration of IO channels is separate from executing it.
Thus, future invocations would look more like this:
Command::new(…).without_io().status().into_result()
src/ios.rs
Outdated
@@ -2,12 +2,14 @@ use std::{ffi::OsStr, io, process::Command}; | |||
|
|||
use crate::{CommandExt, IntoResult}; | |||
|
|||
pub fn command<T: AsRef<OsStr>>(path: T) -> Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature here could be
pub fn command<T: AsRef<OsStr>>(path: T) -> Command { | |
pub fn commands<T: AsRef<OsStr>>(path: T) -> impl Iterator<Item = Command> { |
Note that Option
implements Iterator
, so single-commands can be wrapped in an option when returning them.
src/lib.rs
Outdated
@@ -131,6 +137,18 @@ pub fn with<T: AsRef<OsStr>>(path: T, app: impl Into<String>) -> io::Result<()> | |||
os::with(path, app) | |||
} | |||
|
|||
/// Get command that opens path with the default application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be mentioned that this is a 'no-run' sibling of open::that(…)
, as opposed to, say, open::with(…)
.
02fce60
to
7f5be89
Compare
I am closing this (somewhat stalled) PR in favor of #66 which incorporates some changes from this PR while rounding out the new The reason for my sudden activity is #65 which made me think that this PR actually contains a solution already, so I'd like to see it come to completion now once and and for all. Thanks for everybody involved - and thanks to |
Adds new
commands
function that returns iterator overstd::Command
. This exposes the commands used bythat
to the callee so that they are able to run the command on their side.Related: #63
Related: helix-editor/helix#5820